Skip to content

Rename rawTransaction to rawTx in the persisted transaction metadata#1624

Merged
vinistevam merged 2 commits intomainfrom
refactor/1080-rename-rawTransaction-rawTx
Sep 6, 2023
Merged

Rename rawTransaction to rawTx in the persisted transaction metadata#1624
vinistevam merged 2 commits intomainfrom
refactor/1080-rename-rawTransaction-rawTx

Conversation

@vinistevam
Copy link
Contributor

Explanation

Rename the rawTransaction property to rawTx in the persisted transaction metadata as part of the unification initiative between the core and extension.

The following task will take place to handle the migration and adoption of the patch/release.

References

Fixes https://github.com/MetaMask/MetaMask-planning/issues/1080

Changelog

@metamask/transaction-controller

  • BREAKING: rename the rawTransaction property to rawTx in the persisted TransactionMeta.

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate

@vinistevam vinistevam force-pushed the refactor/1080-rename-rawTransaction-rawTx branch from 32f5491 to 7aa30a7 Compare August 24, 2023 08:58
@vinistevam vinistevam marked this pull request as ready for review August 24, 2023 08:58
@vinistevam vinistevam requested a review from a team as a code owner August 24, 2023 08:58
matthewwalsh0
matthewwalsh0 previously approved these changes Aug 24, 2023
@legobeat
Copy link
Contributor

legobeat commented Aug 25, 2023

What is the motivation for doing the change here, as opposed to doing the inverse change in downstream extension?

Second, if this is indeed the way to go, would it be feasible to expose the same identical value as both names? AIUI this is a pretty significant API-breakage and doing it this way would allow an easier and safer upgrade path for downstreams, where full deprecation can happen at a later point.

@vinistevam
Copy link
Contributor Author

What is the motivation for doing the change here, as opposed to doing the inverse change in downstream extension?

Second, if this is indeed the way to go, would it be feasible to expose the same identical value as both names? AIUI this is a pretty significant API-breakage and doing it this way would allow an easier and safer upgrade path for downstreams, where full deprecation can happen at a later point.

Thanks for bringing that up, the decision was mainly motivated to mitigate risk as the extension is using rawTx and in comparison with mobile would it be just doing the migration and renaming in a few places on the other hand in the extension it would be a much bigger change.
I see your point of reducing even more risk by exposing both of the values as a first phase and only after doing the shift. I will bring that to the team.

matthewwalsh0
matthewwalsh0 previously approved these changes Aug 30, 2023
@vinistevam vinistevam force-pushed the refactor/1080-rename-rawTransaction-rawTx branch from 871cd1c to 4cfe84b Compare September 1, 2023 08:48
@vinistevam vinistevam merged commit e197502 into main Sep 6, 2023
@vinistevam vinistevam deleted the refactor/1080-rename-rawTransaction-rawTx branch September 6, 2023 08:48
vinistevam added a commit to MetaMask/metamask-mobile that referenced this pull request Sep 26, 2023
…rawTx (#7264)

This PR aims to adopt the changes in core renaming `rawTransation` to
`rawTx`. The change is part of an effort to align the extension and core
controller towards unification.
- Generated the patch with the changes to rename  the properties
- Add migration and tests to properly rename 

This was merged in the TransactionController in the core repo by PR MetaMask/core#1624 .
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants